feat: WIP - upgrade resource and did modules to v0.50.x #876
Conversation
…into prathyusha/upgrade_resource
…into prathyusha/upgrade_resource
* wip * wip * wip * wip * feat(params): add migrator support to did module * feat: add autocli support * wip * fix * wip * chore: remove comment * chore: add migrate * chore: add migration in module.go * feat: add SetParams function * go.mod
…into prathyusha/upgrade_resource
…into prathyusha/upgrade_resource
…d/cheqd-node into prathyusha/upgrade_resource
PrathyushaLakkireddy
changed the title
Eengineer1
changed the title
|
|
||
| // Apply changes. We create a new version on deactivation to track deactivation time | ||
| err = k.AddNewDidDocVersion(&ctx, &didDoc) | ||
| err = k.AddNewDidDocVersion(&goCtx, &didDoc) | ||
| if err != nil { | ||
| return nil, types.ErrInternal.Wrapf(err.Error()) |
There was a problem hiding this comment.
Is this non-constant string check necessary? If so, update to the necessary syntax / format.
x/did/types/params_legacy.go
Outdated
| var ( | ||
| KeyCreateDid = []byte("KeyCreateDid") | ||
| KeyUpdateDid = []byte("UpdateDid") | ||
| KeyDeactivateDid = []byte("DeactivateDid") | ||
| KeyBurnFactor = []byte("BurnFactor") | ||
| ) |
There was a problem hiding this comment.
| var ( | |
| KeyCreateDid = []byte("KeyCreateDid") | |
| KeyUpdateDid = []byte("UpdateDid") | |
| KeyDeactivateDid = []byte("DeactivateDid") | |
| KeyBurnFactor = []byte("BurnFactor") | |
| ) | |
| var ( | |
| KeyCreateDid = []byte("FeeParamsCreateDid") | |
| KeyUpdateDid = []byte("FeeParamsUpdateDid") | |
| KeyDeactivateDid = []byte("FeeParamsDeactivateDid") | |
| KeyBurnFactor = []byte("FeeParamsDidBurnFactor") | |
| ) |
There was a problem hiding this comment.
Let's make the notation consistent as suggested above 👍.
There was a problem hiding this comment.
Are we restoring unit tests soon?
Let's validate the changes are functional, before progressing any further.
x/resource/genesis.go
Outdated
| feeParams := k.GetParams(ctx) | ||
| feeParams, err := k.GetParams(ctx) | ||
| if err != nil { | ||
| panic(fmt.Sprintln("Cannot get fee params: %s", err.Error())) |
There was a problem hiding this comment.
Again, as denoted: Let's either replace with a valid syntactical format or skip the lint rule.
| if req == nil { | ||
| return nil, status.Error(codes.InvalidArgument, "invalid request") | ||
| } | ||
|
|
||
| ctx := sdk.UnwrapSDKContext(c) | ||
| // ctx := sdk.UnwrapSDKContext(c) |
There was a problem hiding this comment.
Let's safely remove this.
x/resource/types/params.go
Outdated
| // Default parameter values | ||
| // const ( | ||
| // DefaultCreateResourceImageFee = 100000 // Example value | ||
| // DefaultCreateResourceJSONFee = 75000 // Example value | ||
| // DefaultCreateResourceDefaultFee = 50000 // Example value | ||
| // DefaultBurnFactor = "0.5" // Example value | ||
| // BaseMinimalDenom = "ncheq" // Example value | ||
| // ) |
There was a problem hiding this comment.
Let's safely remove this as it's already defined in genesis.go.
x/resource/types/params_legacy.go
Outdated
| var ( | ||
| KeyResourceImageFee = []byte("KeyResourceImageFee") | ||
| KeyCreateResourceJSONFee = []byte("KeyCreateResourceJSONFee") | ||
| KeyCreateResourceKeyFee = []byte("KeyCreateResourceKeyFee") | ||
| KeyBurnFactor = []byte("KeyBurnFactor") | ||
| ) |
There was a problem hiding this comment.
| var ( | |
| KeyResourceImageFee = []byte("KeyResourceImageFee") | |
| KeyCreateResourceJSONFee = []byte("KeyCreateResourceJSONFee") | |
| KeyCreateResourceKeyFee = []byte("KeyCreateResourceKeyFee") | |
| KeyBurnFactor = []byte("KeyBurnFactor") | |
| ) | |
| var ( | |
| KeyResourceImageFee = []byte("FeeParamsResourceImage") | |
| KeyCreateResourceJSONFee = []byte("FeeParamsResourceJSON") | |
| KeyCreateResourceKeyFee = []byte("FeeParamsResourceKeyFee") | |
| KeyBurnFactor = []byte("FeeParamsResourceBurnFactor") | |
| ) |
There was a problem hiding this comment.
Let's make the notation consistent as suggested above 👍.
|
Let's focus on resolving the build errors, before moving on with unit, integration + upgrade tests in that order. |
There was a problem hiding this comment.
Why is the content removed but not the file?
I do see you've added it to .gitignore. What's the modified approach here?
| func (k Keeper) SetDidDocVersion(ctx context.Context, value *types.DidDocWithMetadata, override bool) error { | ||
| hasdidVersion, err := k.HasDidDocVersion(ctx, value.DidDoc.Id, value.Metadata.VersionId) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !override && hasdidVersion { |
There was a problem hiding this comment.
What if override takes precedence over checking whether there's a DID doc version or not?
In this scenario, HasDidDocVersion will not be invoked at all, if override is true. That's not reflected in this refactored block of code.
PS: If A && B {}, B will be checked if A resolves to true. Otherwise, B won't be invoked / evaluated at all as the whole statement resolves to false if A is false.
| if err := msg.ValidateBasic(); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Isn't basic validation performed by the message server as is or has that significantly changed?
If yes for the former, do we need this extra validation step? My guess is we don't.
| denoms := msg.Amount.Denoms() | ||
| if len(denoms) != 0 { | ||
| err := ValidateDenom(denoms, bondDenom) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| err := msg.ValidateBasic() | ||
| err = msg.ValidateBasic() |
There was a problem hiding this comment.
Is it necessary to re-run basic validation here as well as at the beginning?
| if err := msg.ValidateBasic(); err != nil { | ||
| return nil, err | ||
| } |
No description provided.